-
Notifications
You must be signed in to change notification settings - Fork 809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MU WPCOM: dashboard: add launchpad #41138
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
cae4a82
to
26a557a
Compare
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
26a557a
to
1a7922f
Compare
fallback: { | ||
...jetpackWebpackConfig.resolve.fallback, | ||
events: require.resolve( 'events/' ), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken these changes from the previous PR: #39806
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it about?
712aee4
to
85f4aed
Compare
f65772b
to
498f09a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99f805c
to
b0b8bfd
Compare
b0b8bfd
to
97088ad
Compare
...ckages/jetpack-mu-wpcom/src/features/wpcom-dashboard-widgets/wpcom-launchpad-widget/index.js
Outdated
Show resolved
Hide resolved
97088ad
to
ad0f173
Compare
A couple things I noticed:
|
@@ -73,6 +76,8 @@ | |||
"@wordpress/url": "4.16.0", | |||
"clsx": "2.1.1", | |||
"debug": "4.4.0", | |||
"events": "^3.3.0", | |||
"i18n-calypso": "7.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is interesting, we have some components that use this, and others that use @wordpress/i18n
. What's the story there? Maybe @jsnajdr knows. How to decide what to use...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think ideally we should switch to @wordpress/i18n
. But I don't want to trigger string changes for this PR. Also, there's some use cases that can't be easily done with @wordpress/i18n
such as strings with HTML elements inside of them (I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's some use cases that can't be easily done with @wordpress/i18n such as strings with HTML elements inside of them (I think).
I think we have createInterpolateElement for that use-case. So I think we're covered there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, looking a that, it does work a bit differently, and the strings would still have to change. I guess that could be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be wrong, but I don't think that translation strings using i18n-calypso
's translate()
function would even be picked up, only the @wordpress/i18n
-style __()
, _x()
, and so on.
We've generally avoided @automattic/components
for similar reasons. It has (or has had in the past, anyway) a lot of stuff that's specific to Calypso that doesn't work so well in Jetpack.
Thanks, I'll investigate! Sure, the query string might be ok in this case. I guess it's fine to reload for this action. |
3fd191d
to
3469f65
Compare
Both addressed :) |
jetpack-downloader doesn't seem to work for me at the moment. |
I still see the empty widget, I don't know if it's jetpack downloader not working properly or if the fix didn't work properly |
So it's definitely jetpack downloader (I don't know why yet) |
* | ||
* @return {Function} A ref callback. | ||
*/ | ||
function useSetHrefBase() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this weird hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So currently some of the links in Launchpad
are relative. These are supposed to be wordpress.com links, so we need to adjust the base. There other wp-admin links are absolute and they are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead have a prop for the component instead (maybe as a follow-up) to make the component calypso agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplication is not ideal, but it should be ok for now.
fe094a0
to
c88dcb0
Compare
c88dcb0
to
2dfa068
Compare
Code Coverage SummaryCoverage changed in 2 files.
|
Let's do it :) |
@@ -9284,6 +9494,10 @@ packages: | |||
[email protected]: | |||
resolution: {integrity: sha512-QADzlaHc8icV8I7vbaJXJwod9HWYp8uCqf1xa4OfNu1T7JVxQIrUgOWtHdNDtPiywmFbiS12VjotIXLrKM3orQ==} | |||
|
|||
[email protected]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
It's not really +1000 LoC! That's just the package lock file. 😊
See Automattic/wp-calypso#95386.
Proposed changes:
See #39806 for some prior work.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: